correctly apply multiple with_prototypes when a context is set #220
Conversation
keith-hall
force-pushed
the
fix_set_with_prototype
branch
from
d4e27a4 to
5885304
Compare
|
You can use a move closure here: (See this blog post) |
src/parsing/parser.rs
Outdated
| // a `with_prototype` stays active when the context is `set` | ||
| // until the context layer in the stack (where the `with_prototype` | ||
| // was initially applied) is popped off. | ||
| let mut proto_ids = old_proto_ids.clone().unwrap_or(Vec::new()); |
There was a problem hiding this comment.
Not sure if it's necessary in this case, but clippy might recommend doing this instead:
unwrap_or_else(|| Vec::new())
The reason is that with unwrap_or, the argument is always constructed, even when the option had a value. With unwrap_or_else, that only happens when it's actually needed.
There was a problem hiding this comment.
that makes sense, it's what I originally wanted to achieve; updated - thanks.
| @@ -1,5 +1,4 @@ | |||
| loading syntax definitions from testdata/Packages | |||
| FAILED testdata/Packages/ASP/syntax_test_asp.asp: 162 | |||
There was a problem hiding this comment.
🎉 very nice!
Have you checked if it happens to fix any others of the syntests?
There was a problem hiding this comment.
it doesn't fix any other tests at the current submodule version, but will allow us to update the sublimehq/Packages submodule and not have failing Java(doc) tests etc.
|
Thanks for that @robinst, it worked perfectly ❤️ |
|
It's good to finally have these bugs fixed :)! |
src/parsing/parser.rs
Outdated
| // a `with_prototype` stays active when the context is `set` | ||
| // until the context layer in the stack (where the `with_prototype` | ||
| // was initially applied) is popped off. | ||
| let mut proto_ids = old_proto_ids.clone().unwrap_or_else(|| Vec::new()); |
There was a problem hiding this comment.
Suppose a set has multiple context refs, this appears to add the old prototypes to each new pushed frame, whereas the old behaviour was to only add the old prototypes to the last pushed frame (see the old else { None } block).
This seems unintended, but maybe it was intended.
There was a problem hiding this comment.
nice spot, it should in fact keep the old prototypes on the first pushed frame, there is no need to duplicate it on the other frames as it will be applied from the first pushed frame anyway. I've fixed this and added a test which verifies that it matches ST's behavior.
|
So after we merge this, we can:
|
|
@keith-hall Do you wanna do the honors of raising a PR to update the packs? |
fixes #178 and fixes #160 and also the ASP syntax test failures.
However, due to my limited Rust knowledge, in it's current state, this PR also breaks something - any help anyone can give me here would be highly appreciated! I temporarily removed
lvl.captures.as_ref()from line 312 ofparser.rs(replaced it withNone) as I was getting a compiler error which I don't know how to fix properly:Does anyone know what I need to do to satisfy the borrow checker please?